Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cmake static or shared #1160

Merged
merged 17 commits into from
Mar 8, 2023

Conversation

autoantwort
Copy link
Contributor

Same as #951 but merged with master

@prince-chrismc
Copy link

This looks really good!

@autoantwort
Copy link
Contributor Author

@prince-chrismc Can you approve the ci again? :)

@prince-chrismc
Copy link

I don't have permission i just would benefit from this PR ❤️

@michael-grunder
Copy link
Collaborator

michael-grunder commented Jan 31, 2023

Thanks for rebasing!

This looks good to me but I'll cc @bjosv and @chayim (our resident CMake experts 😄) in case anything catches their eye.

Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this PR makes it look a lot cleaner.

examples/CMakeLists.txt Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing, the option BUILD_SHARED_LIBS seems already to be defaulted to ON.
The CI needs to be changed in that case.
(Also found a legacy nit regarding filename)

Co-authored-by: Bjorn Svensson <bjorn.a.svensson@est.tech>
@michael-grunder
Copy link
Collaborator

@autoantwort @chayim Assuming there are no objections, I'll probably squash and merge this PR since there are so many commits.

Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@@ -45,50 +43,25 @@ IF(WIN32)
ADD_DEFINITIONS(-D_CRT_SECURE_NO_WARNINGS -DWIN32_LEAN_AND_MEAN)
ENDIF()

ADD_LIBRARY(hiredis_static STATIC ${hiredis_sources})
ADD_LIBRARY(hiredis::hiredis_static ALIAS hiredis_static)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you have removed this you have broke the possibility of having both shared and static libraries installed and exported to hiredis-targets.cmake. Existing projects which specify hiredis::hiredis_static in TARGET_LINK_LIBRARIES would be broken and if the target will be changed to hiredis::hiredis they will be linked to shared library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that is true. I could add the following (behind an option)

set_target_properties(hiredis PROPERTIES EXPORT_NAME hiredis_static)

then you have your hiredis::hiredis_static target back.

Copy link

@dmitrmax dmitrmax Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Go ahead please.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JFYI, there is a brilliant article for libraries' authors about exporting shared/static configurations in CMake.

https://alexreinking.com/blog/building-a-dual-shared-and-static-library-with-cmake.html

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No rush @autoantwort but if you add the final change to allow building both I'll get this merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-grunder michael-grunder merged commit e9243d4 into redis:master Mar 8, 2023
@michael-grunder
Copy link
Collaborator

Merged, thanks!

arnout pushed a commit to buildroot/buildroot that referenced this pull request Jul 27, 2023
Removed patch which is included in this release.

Changelog: https://github.com/redis/hiredis/blob/master/CHANGELOG.md

This release includes improvements for static-only builds:
redis/hiredis#1160

Signed-off-by: Bernd Kuhls <bernd@kuhls.net>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
citral23 pushed a commit to citral23/buildroot that referenced this pull request Sep 18, 2023
Removed patch which is included in this release.

Changelog: https://github.com/redis/hiredis/blob/master/CHANGELOG.md

This release includes improvements for static-only builds:
redis/hiredis#1160

Signed-off-by: Bernd Kuhls <bernd@kuhls.net>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants